-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix commands: criteria, layout, move, workspace #2392
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, didn't see this was still WIP
sway/criteria.c
Outdated
return NULL; | ||
} | ||
size_t id = view->swayc->id; | ||
int len = snprintf(NULL, 0, "%zu", id) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the standard way to compute the string's length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's not a length (as in number of chars), it's a size (as in number of bytes that will get malloc'ed) so it probably makes more sense to name it id_size
. Also it should be a size_t
, not an int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it should be a size_t, not an int.
*printf returns an int and so it makes sense to leave it as it is. It really is splitting hairs, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I found that function after using snprintf like that myself, but for integer string length conversion we have this in common/util.c
(declared in include/util.h
):
int numlen(int n) {
if (n == 0) {
return 1;
}
return log10(n) + 1;
}
(It doesn't handle negative integers nor the full range of size_t, but both are fixable by just changing the prototype of the function. We only have one user though, it's a shame but it might be just as well to remove that function...)
sway/sway.5.scd
Outdated
|
||
*move* left|right|up|down [<px>] | ||
*layout* toggle [split|tabbed|stacking|splitv|splith] [split|tabbed|stacking|splitv|splith]... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code doesn't seem to handle split
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, thanks.
Can we work on this a few commands at a time? I'd rather not do a mega-PR which attempts to close #2336 all in one go. |
Got it, I'll add a checklist in the description |
3272167
to
750f38c
Compare
Ready for review, though not sure if I've missed any edge cases in testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: split lines so they don't go past column 80.
sway/tree/workspace.c
Outdated
return current_workspace; | ||
|
||
char *name_cpy = strdup(name); | ||
char *first_word = strtok(name_cpy, " "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for getting and checking against the first word in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it to consolidate logic, treating commands '[move container to] workspace next thing' as just the next workspace without having to check for each case again in their respective functions. However, thinking about it, I guess it overcorrects if the name is quoted: 'workspace "next thing"' also gets treated as the next workspace. So I'm not opposed to changing it.
sway/tree/workspace.c
Outdated
ws = current_workspace; | ||
} else if (strcasecmp(first_word, "back_and_forth") == 0) { | ||
if (prev_workspace_name) { | ||
ws = container_find(&root_container, _workspace_by_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this recreate the workspace if it doesn't exist anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's recreated in the commands that call it, but I wasn't sure whether it should be recreated in all cases (there's a few other functions that call it), so I didn't.
Just needs the long lines split and then it will LGTM. |
The logic is all done now, I think; I'll have a look at splitting the long lines later, and maybe squash some commits together. |
Allow optional --no-auto-back-and-forth flag, as well as refactoring some logic
Thanks! |
Ref #2336
Fixes #1518
Criteria:
Commands:
Also prevents renaming workspace to "back_and_forth", as well as making it clear that they cannot be named to "special" workspace names.